-
Notifications
You must be signed in to change notification settings - Fork 57
Move filter graph to stand alone class #831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pushed a fix for the extra |
Latest CI errors don't seem to be related with the PR. 2 errors:
and
I believe the second error is addressed already in #847, so I will need to rebase. Not sure if the first one is addressed. @scotts : should I rebase now or you need time to fix 1st issue? |
Rebased on top of latest master. I hope this will help ci to pass as some fixes for ci has recently landed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @dvrogozh , this looks great! I made a few comments below from a first pass, LMK what you think.
AVRational inputAspectRatio = {0, 0}; | ||
int outputWidth = 0; | ||
int outputHeight = 0; | ||
AVPixelFormat outputFormat = AV_PIX_FMT_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, in main
, we are hard-cording the output format to AV_PIX_FMT_RGB24
. Do we expect the outputFormat
to be used differently in the future (maybe in the CUDA 10bit PR?) ?
I'm asking because if not, maybe we should still hard-code the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect the
outputFormat
to be used differently in the future (maybe in the CUDA 10bit PR?) ?
Yes, it will be different for HW filters:
- For CPU,
outputFormat = AV_PIX_FMT_RGB24
(or other output format if needed) - For CUDA,
outputFormat = AV_PIX_FMT_CUDA
(and actual format will be set via filter option:scale_cuda=format=nv12
) - For XPU VAAPI,
outputFormat = AV_PIX_FMT_VAAPI
(and actual format will be set via filter option:scale_vaapi=format=rgba
)
FFmpeg filter graphs allow to cover a lot of use cases including cpu and gpu usages. This commit moves filter graph support out of CPU device interface which allows flexibility in usage across other contexts. Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
@NicolasHug, I've addressed your comments. One thing worth noting is that I've introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swsFrameContext.inputWidth = avFrame->width; | ||
swsFrameContext.inputHeight = avFrame->height; | ||
swsFrameContext.inputFormat = frameFormat; | ||
swsFrameContext.outputWidth = expectedOutputWidth; | ||
swsFrameContext.outputHeight = expectedOutputHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we should prefer using aggregate initialization here, instead of setting each field individually. I think I have a preference for aggregate initialization because if we add a new field to the SwsFrameContext
struct, we'd get a loud compilation error if we forget to initialize the field (which is good).
@scotts any pref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer objects to be fully specified on construction, rather than default-constructed and then members set one-by-one. But I don't think aggregate initialization will save use here from forgetting a field, as I think that for types with a default, it will just use that.
What I think we should do is actually just create a constructor for all of our structs. And to make sure we don't miss any members, we should remove the default constructor. (When possible. If we're creating an array of something we won't be able to do that.)
|
||
std::stringstream filters; | ||
filters << "scale=" << expectedOutputWidth << ":" << expectedOutputHeight; | ||
filters << ":sws_flags=bilinear"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that we now have to re-create the string for every single frame in order to compare the filtersContext
objects. Maybe eventually we should separate the concerns between filter-graph creation parameters and comparison operators. But I guess for now this is cheap enough.
Changes:
CpuDeviceInterface
)These changes will allow:
See the following draft PR with the example of using the defined class for ffmpeg-vaapi backend to enable Intel GPU support:
I believe it can be used as a close reference to implement similar support for ffmpeg-cuda filters (
scale_cuda
,scale_npp
).CC: @scotts @NicolasHug @eromomon